fix(proxyabl): close trailing-slash scope-check bypass in middleware#36
Open
davidcrowe wants to merge 1 commit into
Open
fix(proxyabl): close trailing-slash scope-check bypass in middleware#36davidcrowe wants to merge 1 commit into
davidcrowe wants to merge 1 commit into
Conversation
createProxyablMiddleware derived the tool name with req.path.split('/').pop(),
which returns an empty string when the path has a trailing slash (or a double
slash). The subsequent `if (tool)` guard then skipped the scope check entirely,
so a request to `/proxy/deploy/` bypassed the scopes configured for `deploy` —
a fail-open in a deny-oriented layer. Derive the tool from the last non-empty
segment so enforcement is invariant to trailing and duplicate slashes. Adds a
regression test that fails against the old code (verified) and passes now.
Note: this does not change the separate policy question of whether tools with
NO configured scopes should be allowed by default (current behavior) — that is
validatabl's deny-by-default domain and is left unchanged here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
createProxyablMiddlewarederives the tool name to scope-check from the request path:A trailing slash (or a double slash) makes the final segment an empty string, the
if (tool)guard is false, and the scope check is skipped entirely. A caller withoutdeploy:writewho requests/proxy/deployis correctly rejected (403) — but the same caller requesting/proxy/deploy/sails through. That's a fail-open in a deny-oriented layer, reachable by appending one character.Fix
Derive the tool from the last non-empty path segment, making enforcement invariant to trailing and duplicate slashes:
Verification
New regression test (
packages/proxyabl/__tests__/middleware.scope.test.ts) drives the middleware through a real gateway context:next()✓/proxy/deploy→ 403 ✓/proxy/deploy/(trailing slash) → 403/proxy//deploy//(double slash) → 403The two slash cases fail against the current code (confirmed by reverting the one-line fix and re-running) and pass with it. Full suite: 18 files / 163 tests green.
Out of scope (flagging for a policy decision)
This does not touch the separate question of whether a tool with no configured scopes should be allowed by default — currently
assertToolScopesreturns early when a tool declares no required scopes, so unknown/unconfigured tools pass. Whether proxyabl should deny unknown tools outright is a deny-by-default policy decision that belongs with validatabl's model, not this bugfix. Happy to open a separate issue if we want to change it.